-
Notifications
You must be signed in to change notification settings - Fork 534
Add webhooks feature to Insight dashboard #6929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6929 +/- ##
=======================================
Coverage 55.49% 55.49%
=======================================
Files 909 909
Lines 58406 58406
Branches 4069 4069
=======================================
Hits 32415 32415
Misses 25886 25886
Partials 105 105
🚀 New features to boost your workflow:
|
size-limit report 📦
|
8887def
to
4b39e26
Compare
...p/(app)/team/[team_slug]/[project_slug]/insight/webhooks/components/create-webhook-modal.tsx
Outdated
Show resolved
Hide resolved
...p/(app)/team/[team_slug]/[project_slug]/insight/webhooks/components/create-webhook-modal.tsx
Outdated
Show resolved
Hide resolved
4b39e26
to
b083bcc
Compare
b083bcc
to
36fcc86
Compare
const handleTestWebhook = async () => { | ||
if (webhookUrl) { | ||
await testWebhookEndpoint(webhookUrl); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testWebhookEndpoint
function requires a type
parameter that's missing in this call. This will cause webhook tests to fail since the API needs to know whether it's an 'event' or 'transaction' type webhook.
Consider updating the function call to include the current filter type from the form:
await testWebhookEndpoint(webhookUrl, form.watch('filterType'));
This ensures the correct webhook type is passed to the testing endpoint.
const handleTestWebhook = async () => { | |
if (webhookUrl) { | |
await testWebhookEndpoint(webhookUrl); | |
} | |
const handleTestWebhook = async () => { | |
if (webhookUrl) { | |
await testWebhookEndpoint(webhookUrl, form.watch('filterType')); | |
} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
toAddresses: z | ||
.string() | ||
.nonempty({ message: "To address is required" }) | ||
.refine((val) => val.split(",").every(isValidAddress), { | ||
message: "Enter a valid address", | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation schema has an inconsistency between addresses
and toAddresses
fields. Both are marked as required with .nonempty()
, but the UI in FilterDetailsStep
only shows one field at a time based on the filterType
.
When filterType
is "transaction", the form will still attempt to validate the hidden addresses
field, causing validation errors for users. Similarly, when filterType
is "event", it will validate the hidden toAddresses
field.
Consider implementing conditional validation based on the filterType
:
export const webhookFormSchema = z.discriminatedUnion('filterType', [
z.object({
filterType: z.literal('event'),
addresses: z.string().nonempty().refine(/*...*/),
// Make toAddresses optional for event type
toAddresses: z.string().optional(),
// other fields...
}),
z.object({
filterType: z.literal('transaction'),
// Make addresses optional for transaction type
addresses: z.string().optional(),
toAddresses: z.string().nonempty().refine(/*...*/),
// other fields...
})
]);
This approach ensures fields are only validated when they're relevant to the selected filter type.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
5a6b34f
to
b7f5b7f
Compare
} catch (error) { | ||
console.error("Error loading project or webhooks", error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling here only logs to the console without providing user feedback. Consider implementing a proper error state UI component that distinguishes between "no webhooks exist" and "failed to load webhooks." This would improve the user experience by clearly communicating when there's a system issue rather than showing an empty state that incorrectly suggests the user has no webhooks configured.
} catch (error) { | |
console.error("Error loading project or webhooks", error); | |
} | |
} catch (error) { | |
console.error("Error loading project or webhooks", error); | |
setError("Failed to load webhooks. Please try again later."); | |
} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
export interface AbiResponse { | ||
abis: Record<string, AbiData>; | ||
errors: Record<string, string>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exported interface 'AbiResponse' is not being used anywhere in the codebase, which is causing the knip linter to fail. Either remove the 'export' keyword to make it a local interface, or remove the interface entirely if it's not needed.
Spotted by Diamond (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
b7f5b7f
to
6e90e96
Compare
.refine((val) => val.split(",").every(isValidAddress), { | ||
message: "Enter a valid address", | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current address validation has a potential issue with empty inputs. The .refine()
method will pass validation for strings containing only commas or spaces (like ",,,") because it's checking if every address is valid, but not verifying that valid addresses actually exist.
Consider enhancing the validation to ensure there's at least one valid address after splitting and filtering:
.refine(
(val) => {
const addresses = val.split(',').map(addr => addr.trim()).filter(Boolean);
return addresses.length > 0 && addresses.every(isValidAddress);
},
{ message: 'Enter at least one valid address' }
)
This approach first trims and filters out empty entries, then verifies both that there's at least one address and that all addresses are valid.
.refine((val) => val.split(",").every(isValidAddress), { | |
message: "Enter a valid address", | |
}), | |
.refine( | |
(val) => { | |
const addresses = val.split(",").map(addr => addr.trim()).filter(Boolean); | |
return addresses.length > 0 && addresses.every(isValidAddress); | |
}, | |
{ message: "Enter at least one valid address" } | |
), |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
@@ -165,7 +165,9 @@ export const MultiSelect = forwardRef<HTMLButtonElement, MultiSelectProps>( | |||
{props.customTrigger || ( | |||
<Button | |||
ref={ref} | |||
{...props} | |||
{...(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this added?
|
||
{/* Navigation Buttons */} | ||
<div className="flex justify-between pt-4"> | ||
{currentStep !== WebhookFormSteps.BasicInfo ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The navigation buttons should also go inside the step component so this is easier to maintain and update later
render={({ field }) => ( | ||
<FormItem className="flex flex-col"> | ||
<div | ||
className={cn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove cn
here - not required, please also remove this from everywhere else as well.
only use it when conditional styles need to be added or when the className string is too large and you want to break it down to multiple lines
})); | ||
}, [pairs, isOpen, addresses, chainIds, type, thirdwebClient]); | ||
|
||
const results = useQueries({ queries }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using useQueries
- If the number of queries can be large here it will crash the page due to a large amount of memory used. If possible, please do all request in a single useQuery
* @param address Address to validate | ||
* @returns Boolean indicating if address is valid | ||
*/ | ||
export const isValidAddress = (address: string): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a isAddress
utility exported from thirdweb/utils
fro this already
|
||
if (!response.ok) { | ||
const errorText = await response.text(); | ||
throw new Error(`Failed to delete webhook: ${errorText}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't throw errors in server actions - You won't be able to see them in client component (you will only be able to see errors in local dev server but not in vercel preview or production environments)
If you want to return error message, you have to return the error message manually
Example:
return {
errorMessage: await response.text()
}
6e90e96
to
5d068e0
Compare
toAddresses: z | ||
.string() | ||
.nonempty({ message: "To address is required" }) | ||
.refine((val) => val.split(",").every(isValidAddress), { | ||
message: "Enter a valid address", | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation schema for toAddresses
currently requires this field unconditionally, but it should only be required when filterType
is set to 'transaction'. For event webhooks, this field isn't applicable. Consider implementing conditional validation using Zod's .superRefine()
or a dynamic schema based on the selected filter type to prevent validation errors when users are creating event webhooks. This would improve the form experience by only validating fields relevant to the current webhook type.
toAddresses: z | |
.string() | |
.nonempty({ message: "To address is required" }) | |
.refine((val) => val.split(",").every(isValidAddress), { | |
message: "Enter a valid address", | |
}), | |
toAddresses: z | |
.string() | |
.optional() | |
.superRefine((val, ctx) => { | |
// Only validate if filterType is 'transaction' | |
if (ctx.parent.filterType === 'transaction') { | |
if (!val || val.trim() === '') { | |
ctx.addIssue({ | |
code: z.ZodIssueCode.custom, | |
message: "To address is required for transaction webhooks", | |
}); | |
return; | |
} | |
if (!val.split(",").every(isValidAddress)) { | |
ctx.addIssue({ | |
code: z.ZodIssueCode.custom, | |
message: "Enter a valid address", | |
}); | |
} | |
} | |
}), |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Add Webhooks Feature to Insight Dashboard
This PR adds a new webhooks feature to the Insight dashboard, allowing users to create, test, and manage webhooks for blockchain events and transactions. The implementation includes:
The PR also includes several UI improvements:
The webhooks feature enables users to receive real-time notifications about on-chain events through their own endpoints, supporting both event and transaction monitoring with customizable filters.
PR-Codex overview
This PR introduces a comprehensive update to the
insight
module, enhancing the webhook functionality with new components and improved layout, while refining user interactions and error handling.Detailed summary
Layout
component for project and team insights.StepIndicator
andBasicInfoStep
for webhook creation.FilterDetailsStep
for advanced webhook settings.ReviewStep
for webhook configuration review.CreateWebhookModal
for creating new webhooks.useTestWebhook
hook.webhooks.ts
for better error handling.Demo
CleanShot 2025-05-07 at 18.55.41.mp4 (uploaded via Graphite)